Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing legacy YAML structures (with new pluggable adapters) #14

Merged
merged 19 commits into from
Dec 4, 2023

Conversation

richardolsson
Copy link
Member

@richardolsson richardolsson commented Dec 3, 2023

At today's hackathon, I worked with Elias on this PR, which introduces a set of pluggable "adapters" to parse messages from either TypeScript messageIds files or YAML files, and translations from YAML files.

This allows for Lyra work both on Zetkin Foundation's legacy repos with messages being defined by the English YAML files, and the new app.zetkin.org repo where type-safe messages are defined in messageIds.ts files.

I'm curious to hear your thoughts on this @amerharb! We used TDD so the PR introduces a bunch of tests, but no CI action for running them.

Configuration

For Lyra to work for both types of repos, we need a configuration file in the repositories, so we designed a simple YAML-based config file. The config file has a list of projects, which allows for future support for mono-repos (which is necessary for Zetkin). For now however, only one project is supported and all but the first will be ignored.

Place the following lyra.yml file in the root of any repo that you want to use Lyra on.

# For app.zetkin.org:
projects:
  - path: .
    messages:
      format: ts
      path: src
    translations:
      format: yaml
      path: src/locale

@amerharb
Copy link
Collaborator

amerharb commented Dec 3, 2023

At today's hackathon, I worked with Elias on this PR, which introduces a set of pluggable "adapters" to parse messages from either TypeScript messageIds files or YAML files, and translations from YAML files.

This allows for Lyra work both on Zetkin Foundation's legacy repos with messages being defined by the English YAML files, and the new app.zetkin.org repo where type-safe messages are defined in messageIds.ts files.

I'm curious to hear your thoughts on this @amerharb! We used TDD so the PR introduces a bunch of tests, but no CI action for running them.

Configuration

For Lyra to work for both types of repos, we need a configuration file in the repositories, so we designed a simple YAML-based config file. The config file has a list of projects, which allows for future support for mono-repos (which is necessary for Zetkin). For now however, only one project is supported and all but the first will be ignored.

Place the following lyra.yml file in the root of any repo that you want to use Lyra on.

# For app.zetkin.org:
projects:
  - path: .
    messages:
      format: ts
      path: src
    translations:
      format: yaml
      path: src/locale

This config looks what we need but shall we call it .lyra.yaml?, I will review it, but I am just wondering if we should merge the other 2 PRs first one need reappove, other have answered a question, or maybe this one include that changes ?

Copy link
Collaborator

@amerharb amerharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good, and I like that we start TDD in this, but I suggest to merge PR12 and 13 first, then if you like I can take care of the conflict.

webapp/.eslintrc.js Outdated Show resolved Hide resolved
webapp/jest.config.js Outdated Show resolved Hide resolved
webapp/src/utils/flattenObject.ts Show resolved Hide resolved
@richardolsson
Copy link
Member Author

Thanks for reviewing! Sure let's merge your PRs first and I can fix the merge conflicts and the other little things that you pointed out in your review. Nice! 💯

This was referenced Dec 4, 2023
@richardolsson
Copy link
Member Author

I've fixed the merge conflicts now, and made some minor changes along the lines of your review. Have look if you want to merge this and continue from this @amerharb!

@richardolsson
Copy link
Member Author

This config looks what we need but shall we call it .lyra.yaml?

To answer this question, I think either name is fine. I wouldn't dig too deep into it now, because I think we need to revisit this later anyway when we're working on multiple projects per lyra instance.

Copy link
Collaborator

@amerharb amerharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just a small thing about naming convention:
TSMessageAdapter -> TsMessageAdapter
YAMLMessage... -> YamlMessage...

@richardolsson
Copy link
Member Author

looks good, just a small thing about naming convention:
TSMessageAdapter -> TsMessageAdapter
YAMLMessage... -> YamlMessage...

Thanks! I personally don't mind either way and can imagine points to be made for both options. I'll go ahead and merge, and if you want to rename them later I won't try to stop you. 😊

@richardolsson richardolsson merged commit f8d07c8 into main Dec 4, 2023
1 check passed
@richardolsson richardolsson deleted the undocumented/legacy-yaml-msg-parsing branch December 4, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants